Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

provide a way to drop a runtime in an async context #2646

Merged
merged 2 commits into from
Jul 21, 2020

Conversation

bdonlan
Copy link
Contributor

@bdonlan bdonlan commented Jul 7, 2020

Motivation

Dropping a runtime normally involves waiting for any outstanding blocking tasks
to complete. When this drop happens in an asynchronous context, we previously
would issue a cryptic panic due to trying to block in an asynchronous context.

Solution

This change improves the panic message, and adds a shutdown_now() function
which can be used to shutdown a runtime without blocking at all, as an out for
cases where this really is necessary.

@bdonlan
Copy link
Contributor Author

bdonlan commented Jul 7, 2020

Failing check: https://dev.azure.com/tokio-rs/Tokio/_build/results?buildId=6133&view=logs&j=edbb8bb1-bc6c-5b89-6b1a-b043f7e49efc

Looks like it didn't even start building for some reason. Can someone please restart it?

@Darksonn Darksonn added A-tokio Area: The main tokio crate C-enhancement Category: A PR with an enhancement or bugfix. M-runtime Module: tokio/runtime labels Jul 7, 2020
@bdonlan
Copy link
Contributor Author

bdonlan commented Jul 7, 2020

Oh wait, I misunderstood the UI. It did hang in test --features full apparently.

@Darksonn
Copy link
Contributor

Darksonn commented Jul 7, 2020

I restarted the windows test.

@Matthias247
Copy link
Contributor

The question here might be how this fits together with whatever the future of structured concurrency is (#1879, #2596, one implementation in #2579 ).

In such a model there shouldn't really be tasks that outlive the runtime anymore since the runtime represents a top level task scope. The runtime would wait in some kind of fashion (either synchronously/thread-blocking) or asynchronously for all tasks to complete. shutdown_now doesn't seem to fit in such a world.

Could your application use a mechanism that sends a cancellation signal to the remainig tasks on the runtime and then waits for it asynchronously to complete, as described in the structured concurrency proposals? That wouldn't require to do a thread-blocking wait on the inner runtime anymore, but instead an await on the runtime scope (which doesn't exist at that point in time).

@bdonlan
Copy link
Contributor Author

bdonlan commented Jul 8, 2020

For context, I created this after observing surprising (to me) behavior when Drop paniced in some tests I had initially wrote for #2645 .

While it would be ideal of course to cancel things cleanly, right now in an async context where a clean cancellation path is unavailable (perhaps tearing things down after a panic or other failure?), our options are:

  1. Enter a blocking context, then call shutdown_timeout(Duration::from_nanos(0)), which doesn't actually block but requires a blocking context
  2. Spawn a thread for the sole purpose of waiting for the runtime to shut down

It seems a bit silly to me that an explicitly non-blocking shutdown should require a blocking context, hence this patch to help rectify this point of confusion.

@carllerche
Copy link
Member

The change to the wait function is good. You are correct that we shouldn't "enter a blocking operation" if the operation doesn't actually block.

Do you think it is worth adding shutdown_now vs. saying to use shutdown_timeout w/ a zero duration?

@bdonlan
Copy link
Contributor Author

bdonlan commented Jul 15, 2020

@carllerche Having whether a function is at all usable in a particular context depend on a numeric parameter seemed a bit sketchy for me, but Id be okay with it either way I suppose if properly documented.

@carllerche
Copy link
Member

My main hesitation w/ shutdown_now is that the name implies to me some difference in the shutdown process. Somehow, shutdown_now() performs the shutdown process more immediately than shutdown(). However, it the shutdown logic is identical, the difference is that the caller does not wait for shutdown to complete.

A more appropriate name could be shutdown_background() or something like that?

Dropping a runtime normally involves waiting for any outstanding blocking tasks
to complete. When this drop happens in an asynchronous context, we previously
would issue a cryptic panic due to trying to block in an asynchronous context.

This change improves the panic message, and adds a `shutdown_blocking()` function
which can be used to shutdown a runtime without blocking at all, as an out for
cases where this really is necessary.
@bdonlan bdonlan force-pushed the async-rt-shutdown branch from e7fc2bd to 56db531 Compare July 16, 2020 21:32
@bdonlan
Copy link
Contributor Author

bdonlan commented Jul 16, 2020

@carllerche Changed to shutdown_background.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-tokio Area: The main tokio crate C-enhancement Category: A PR with an enhancement or bugfix. M-runtime Module: tokio/runtime
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants